Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dcnm_fabric (Ready for review) #286

Merged
merged 232 commits into from
May 6, 2024
Merged

dcnm_fabric (Ready for review) #286

merged 232 commits into from
May 6, 2024

Conversation

allenrobel
Copy link
Collaborator

Opening a PR to run dcmn_fabric through our sanity tests and fix whatever issues these reveal.

This module is working for deleted, merged, and query states.

No additional major changes are expected at this point.

This is a work in progress.  merged state works, but it still needs a lot of cleaning up and support for additional fabric parameters.

TODO:

general cleanup
query state
unit tests
integration tests
No dynamic template capability in this commit.
A Results() class is introduced.  The intent is to:

1. Standardize results processing and output
2. Rremove the distraction of results processing from the main logic of a given module.
3. Enable result format changes to be made in a single place, should we ever want to do that.
Add class-level doc strings with Usage examples to the following classes:

- FabricCreateBulk
- FabricDelete
- FabricQuery
- FabricUpdateBulk
- Results
Previously, results contained an "OK" and "FAILED" key for each of results, response, diff.

Results for existing modules do not have this.  So, we've changed the results for dcnm_fabric to more closely match the results of existing modules.

We've still retained the unique sequence_number for each of result, response, and diff entries so that they can be correlated.

Also:

Move template_parse_each_fabric.py from module_utils/fabric to module_utils/fabric/vxlan
Change the import for TemplateParseEasyFabric
RestSend and Results are now generic enough to use across all of dcnm_image_upgrade, dcnm_image_policy, and dcnm_fabric modules.

Hence, we can use module_utils/common/rest_send.py for dcnm_fabric and delete rest_send_fabric.py/
- Add unit tests for:
-- FabricQuery()
-- FabricCreate()

- Copy the following from dcnm_image_upgrade/plugins/module_utils/common
-- rest_send.py
-- results.py
- test_fabric_create_bulk.py: Update docstrings for a few tests
- update.py: FabricUpdateCommon._send_payloads(), fix reference to Results() property that no longer exists
- query.py: run thru black
- fabric_summary.py: use a private copy of Results()
- fabric_details.py: use a private copy of Results()
- delete.py: run thru black
- create.py: add type hinting to a few methods
- common.py: fix bad f-string interpolation
FabricCreateCommon() and FabricUpdateCommon(): move _fixup_payloads_to_commit() to FabricCommon() and modify it to raise ValueError if ANYCAST_GW_MAC is not convertable.

test_fabric_update_bulk.py: Add unit test to verify behavior when ANYCAST_GW_MAC is not convertable.

test_fabric_update_bulk.py: Modify unit test 00031 to include ANYCAST_GW_MAC that IS convertable.
FabricCreate().commit(): Leverage FabricCreateCommon()._set_fabric_create_endpoint()

FabricCreate().commit(): Set RestSend().check_mode and RestSend().timeout prior to sending the request.
Breakup assert statements into sections for visual consistency across test cases.
FabricUpdateBulk: Send payload only when it would change the controller fabric config.

FabricUpdate: Remove this class for now since it's not used.
module_utils/fabric/ruleset.py: TODO: We still need to handle the case where "and" and "or" are present in a rule.  This impacts handling of one rule in the Easy_Fabric template.  Specifically for ANYCAST_RP_IP_RANGE:

Rule:

( STATIC_UNDERLAY_IP_ALLOC == ' False' and UNDERLAY_IS_V6 == ' False' and REPLICATION_MODE == 'Multicast' ) or ( STATIC_UNDERLAY_IP_ALLOC == ' True' and UNDERLAY_IS_V6 == ' False' and REPLICATION_MODE == 'Multicast' and RP_MODE == 'bidir' )
Guard against deploying fabrics with either DEPLOYMENT_FREEZE == True or IS_READ_ONLY == True.

Previously, we were collecting fabrics to be config-saved and config-deployed prior to pushing their config changes.  And we were not checking DEPLOYMENT_FREEZE or IS_READ_ONLY values.

Changes for the above entailed:

1. Don't collect fabrics to be config-deployed prior to pushing their new configs (since the new configs may enable either DEPLOYMENT_FREEZE or IS_READ_ONLY).

2. Instead, we now do the following:

a. push the configs for all fabrics that have changes.
b. config-save the fabrics
c. config-deploy each fabric only after refreshing FabricDetailsByName() and verifying that both DEPLOYMENT_FREEZE and IS_READ_ONLY are False.

3. Update all test cases to reflect the above changes.

4. FabricConfigDeploy(): Add properties for fabric_details and fabric_summary so that all verifications regarding whether a fabric is deployable are done within this class.
We've standardized the following:
   - _build_properties() only populates an existing properties dict.
   - _init_properties() initializes a properties dict (and potentially populates it).
1. FabricUpdateBulk(): remove unneeded testcase.  Update docstrings.

2. FabricReplacedBulk(): improve _translate_payload_for_comparison() testcase

3. FabricCommon(): rename _prepare_anycast_gw_mac_for_comparison() to translate_anycast_gw_mac()

4. FabricCommon(): add testcase for translate_anycast_gw_mac()

5. FabricReplacedBulk(): use renamed translate_anycast_gw_mac() from 3 above.

6. ParamInfo(): remove debug statement

7. FabricCommon(): remove unused ControllerResponseError import.

8. FabricCommon(): remove unused code.

9. FabricCommon(): relocate _fixup_anycast_gw_mac() closer to where it is used.
Due to issues found while writing integration tests, am modifying behavior of:

FabricConfigSave()
FabricConfigDeploy()

There were two problems with these:

1. They were called regardless of the value of the value of the playbook DEPLOY parameter.

2. They were not granular in their intepretation of "failed" when setting results.

To solve 1 above, we now pass the payload to each class, rather than just the fabric_name.  If DEPLOY is False (or missing), both classes set result to success and provide a message indicating that config-save, config-deploy were skipped due to the DEPLOY setting.

To solve 2 above, we track (within each class) what specifically caused config-save or config-deploy to be skipped.  If these were skipped due to errors, we set results accordingly and provide a message describing the failure in the response.

All unit tests are updated to reflect the above changes.
Also:

- dcnm_fabric_deleted_basic.yaml: Add a DESCRIPTION section
- dcnm_fabric_deleted_basic.yaml: Fix dcnm_fabric_name_* numbering in comments.
1. FabricConfigSave().commit(): An earlier commit changed FabricConfigSave() to accept the payload via a payload property.  In commit(), the guard conditional was not also changed to test that payload is set.  Fixed.

2. 1. FabricConfigSave(): Fix usage in docstring.

3. FabricConfigDeploy(): Fix docstrings.

4. dcnm_fabric.py Merged().commit(): Fix docstring.

5. test_fabric_config_save.py: Update unit tests to reflect change in 1 above.
…into dcnm_fabric

# Conflicts:
#	tests/sanity/ignore-2.13.txt
#	tests/sanity/ignore-2.14.txt
1. Improved error message explain why the user might have encountered the error.

Changed from:

"Parameter {value} not found in self.info."

To:

"Parameter {value} not found in fabric template. This likely means that the parameter {value} is not appropriate for the fabric type."

2. Add a unit test for the above change.
… it.

Caught in integration tests.  The playbook below would fail, as it should.  BUT for the wrong reason.  The reason given was:

"ParamInfo.parameter: Parameter UNDERLAY_IS_V6 not found in fabric template. This means that either the parameter UNDERLAY_IS_V6, or a parameter that is dependent on UNDERLAY_IS_V6, is not appropriate for the fabric type."

The actual reason is that SUBNET_TARGET_MASK is not a valid parameter for LAN_CLASSIC fabric.

The reason that the message is wrong is because RuleSet() was not resetting self.ruleset when refresh() was called.  For the scenario below, this resulted in the following:

1. RuleSet() was updated for VXLAN_EVPN fabric template and the VXLAN_EVPN params were validated.

2. RuleSet() was updated for LAN_CLASSIC fabric template (but not initialized/result, so it still contained the keys from VXLAN_EVPN fabric, which include SUBNET_TARGET_MASK).

As a result of 2, SUBNET_TARGET_MASK parameter in the LAN_CLASSIC fabric playbook config was matched against the remnents of the VXLAN_EVPN template, which resulted in the above misleading error.

The fix is to reset the ruleset in RuleSet().refresh() prior to updating it with the new template.

We now get the correct error for the playbook below, which is:

"FabricUpdateBulk._fabric_needs_update_for_merged_state: Invalid key: SUBNET_TARGET_MASK found in payload for fabric LAN_CLASSIC_Fabric"

    tasks:
        - cisco.dcnm.dcnm_fabric:
            state: merged
            config:
            -   FABRIC_NAME: VXLAN_EVPN_Fabric
                FABRIC_TYPE: VXLAN_EVPN
                BGP_AS: 65535
                DEPLOY: false
            -   FABRIC_NAME: LAN_CLASSIC_Fabric
                FABRIC_TYPE: LAN_CLASSIC
                IS_READ_ONLY: false
                SUBNET_TARGET_MASK: 31
                BOOTSTRAP_ENABLE: false
                DEPLOY: false

Also, cleaned up pylint line-too-long in debug message.
Make the code a bit easier to read by initializing fabric_name and fabric_type vars at the top of the method, rather than pulling these out of the want dict within the method's code.
@allenrobel allenrobel changed the title dcnm_fabric (Don't merge. Needs more UT/IT work) dcnm_fabric (Ready for review) May 4, 2024
@@ -0,0 +1,733 @@
#!/usr/bin/python
#
# Copyright (c) 2020-2022 Cisco and/or its affiliates.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update Copyright 2024

---
module: dcnm_fabric
short_description: Create Fabrics.
version_added: "0.9.0"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update version_added

- vrf_lite_autoconfig must be set to 1
- AUTO_SYMMETRIC_VRF_LITE must be set to True
- AUTO_VRFLITE_IFC_DEFAULT_VRF must be set to True
- NDFC GUI label, Auto Deploy Default VRF for Peer
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor comment but maybe ensure NDFC GUI label and NDFC GUI tab is at the end of each description? It is in most cases but there are a few options that are not consistent. It's my OCD kicking in :)

That's a nice touch BTW to add this information.

# logging.config.dictConfig and must not log to the console.
# For an example configuration, see:
# $ANSIBLE_COLLECTIONS_PATH/cisco/dcnm/plugins/module_utils/common/logging_config.json
enable_logging = False
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand correctly today, we have to change the code here to enable logging.

enable_logging = True

Wondering if we could also support an ENV variable to enable logging so we don't have to modify the code directly?

log = Log(ansible_module)
if enable_logging is True:
collection_path = (
"/Users/arobel/repos/collections/ansible_collections/cisco/dcnm"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't forget to remove hardcoded path

@mikewiebe mikewiebe merged commit bf14b88 into develop May 6, 2024
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Day 0 Easy fabric provisioning using ansible
2 participants